-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
get latest redirects based on nav-data not version metadata #2563
Conversation
The latest updates on your projects. Learn more about Vercel for Git βοΈ
|
π¦ Next.js Bundle AnalysisThis analysis was generated by the next.js bundle analysis action π€ This PR introduced no changes to the javascript bundle π |
60b98d2
to
2c6abbd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may be overlooking something here, but why is this a .ts
file while the other two are .js
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'd prefer to use TS in both places... but we import the redirects
stuff into next.config.js
, so it needs to be JS rather than TS. Tests can be TS since they don't need to run directly in that context π
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! Yeah that makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM π
Not going to hold the review up over that comment - was just curious.
build-libs/__tests__/get-latest-content-sha-for-product.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with this change, we might as well go to the source of truth rather than having a redirects in our logic.
Co-authored-by: Heat Hamilton <[email protected]>
π Relevant links
ποΈ What
This PR avoids using the
ref
fromversion-metadata
in order to fetch the "latest" redirects from content source repositories.π€· Why
This work is in service of [DEVDOT-???] Release branches as docs single source of truth. At present, our reliance on
version-metadata
is out of convenience, and assumes thatversion-metadata
will have itsref
andsha
updated on every content upload, even when there have been zero changes to version metadata. This method of constant updates seems to be causing issues that outweigh the convenience of this approach.π οΈ How
This PR refactors
getLatestContentRefForProduct
, which upstream fetches the latestref
fromversion-metadata
, intogetLatestContentShaForProduct
, which now fetches the latestsha
from knownnav-data
endpoints.π§ͺ Testing
Tests have been added to assert that
redirects
are successfully fetched with this approach. In addition, we have a more ephemeral demo, formatted as a test file for convenience, that asserts that the redirects fetched using thesha
fromnav-data
are identical to redirects fetched using theref
fromversion-metadata
:π Anything else?
Not at the moment!